[FAU-414] Add support for campo keys #7
Conversation
amiut
changed the title
There was a problem hiding this comment.
I really like Campo Keys, which is part of our domain model, as it should be.
What do you think if we remove unrequired getters and setters. I could imagine something like
Index: src/Infrastructure/Repository/CampoKeysRepository.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/Infrastructure/Repository/CampoKeysRepository.php b/src/Infrastructure/Repository/CampoKeysRepository.php
--- a/src/Infrastructure/Repository/CampoKeysRepository.php (revision 4a72cbaf0318d8804397f2d94b6884a695bbd7af)
+++ b/src/Infrastructure/Repository/CampoKeysRepository.php (date 1716362430281)
@@ -25,8 +25,6 @@
public function degreeProgramCampoKeys(DegreeProgramId $degreeProgramId): CampoKeys
{
- $campoKeys = CampoKeys::empty();
-
/** @var WP_Error|array<WP_Term> $terms */
$terms = wp_get_post_terms(
$degreeProgramId->asInt(),
@@ -34,8 +32,10 @@
);
if ($terms instanceof WP_Error) {
- return $campoKeys;
+ return CampoKeys::empty();
}
+
+ $map = [];
foreach ($terms as $term) {
$campoKey = (string) get_term_meta($term->term_id, self::CAMPOKEY_TERM_META_KEY, true);
@@ -50,10 +50,10 @@
continue;
}
- $campoKeys->set($campoKeyType, $campoKey);
+ $map[$campoKeyType] = $campoKey;
}
- return $campoKeys;
+ return CampoKeys::fromArray($map);
}
/**
Index: src/Domain/CampoKeys.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/Domain/CampoKeys.php b/src/Domain/CampoKeys.php
--- a/src/Domain/CampoKeys.php (revision 4a72cbaf0318d8804397f2d94b6884a695bbd7af)
+++ b/src/Domain/CampoKeys.php (date 1716362430285)
@@ -4,8 +4,9 @@
namespace Fau\DegreeProgram\Common\Domain;
-use InvalidArgumentException;
-
+/**
+ * @psalm-type CampoKeysMap = array<value-of<self::SUPPORTED_CAMPO_KEYS>, string>
+ */
final class CampoKeys
{
public const SCHEMA = [
@@ -46,85 +47,50 @@
private const HIS_CODE_DELIMITER = '|';
- /** @var array<string, string> */
- private array $map = [];
-
- private function __construct()
- {
+ private function __construct(
+ /**
+ * @var CampoKeysMap $map
+ */
+ private array $map
+ ) {
}
public static function empty(): self
{
- return new self();
+ return new self([]);
}
/**
- * @psalm-param array<string, string> $data
+ * @param CampoKeysMap $map
*/
- public static function fromArray(array $data): self
+ public static function fromArray(array $map): self
{
- $instance = new self();
-
- foreach ($data as $key => $value) {
- $instance->set($key, $value);
- }
-
- return $instance;
+ return new self($map);
}
public static function fromHisCode(string $hisCode): self
{
$parts = explode(self::HIS_CODE_DELIMITER, $hisCode);
+ $map = [];
- $instance = new self();
if (isset($parts[0])) {
- $instance->set(DegreeProgram::DEGREE, $parts[0]);
+ $map[DegreeProgram::DEGREE] = $parts[0];
}
if (isset($parts[1])) {
- $instance->set(DegreeProgram::AREA_OF_STUDY, $parts[1]);
+ $map[DegreeProgram::AREA_OF_STUDY] = $parts[1];
}
if (isset($parts[6])) {
- $instance->set(DegreeProgram::LOCATION, $parts[6]);
+ $map[DegreeProgram::LOCATION] = $parts[6];
}
-
- return $instance;
- }
-
- public function set(string $key, string $value): self
- {
- if (! in_array($key, self::SUPPORTED_CAMPO_KEYS, true)) {
- throw new InvalidArgumentException('Unsupported field key.');
- }
-
- $this->map[$key] = $value;
- return $this;
- }
-
- public function degree(): ?string
- {
- return $this->get(DegreeProgram::DEGREE);
- }
-
- public function areaOfStudy(): ?string
- {
- return $this->get(DegreeProgram::AREA_OF_STUDY);
- }
- public function studyLocation(): ?string
- {
- return $this->get(DegreeProgram::LOCATION);
- }
-
- public function get(string $key): ?string
- {
- return $this->map[$key] ?? null;
+ return new self($map);
}
/**
- * @return array<string, string>
+ * @return CampoKeysMap
*/
public function asArray(): array
{
| } | ||
|
|
||
| $term = $this->findTermByCampoKey($taxonomy, $campoKey); | ||
| $result[$taxonomy] = ! is_null($term) ? $term->term_id : 0; |
There was a problem hiding this comment.
I would not add an empty value to the result.
There was a problem hiding this comment.
the problem is if we don't pass 0 term id it will continue finding results with wrong HIS codes.
There was a problem hiding this comment.
I meant something like
if ($term instanceof \WP_Term) {
$result[$taxonomy] = $term->term_id;
}Does it work?
There was a problem hiding this comment.
the problem still remains the same, consider a scenario where editor passes an HIS code like 185|80
but there is no degree with campo key 185 (a.k.a the $term here is null) and as a result the tax query item for degree isn't built here so the tax query doesn't include degree at all and will lead to wrong results, while it shouldn't show anything if a part of HIS code is wrong.
There was a problem hiding this comment.
Oh, okay. So, we create an invalid WP_Query to enforce empty results. We could throw and catch at a higher level the exception instead. On the other side, we don't validate other filters/terms IDs... Could you add a comment to explain this?
There was a problem hiding this comment.
sure that makes more sense, I followed the exception approach with a clarifying comment.
There was a problem hiding this comment.
Thanks for working on this! Left a couple of minor comments. Also, do I understand correctly that naming-wise, you consider an HIS code to consist of multiple Campo Keys? So Campo Keys are, e.g., "08", "185", or "AB1", and an HIS code would be, e.g., "08|185|-|-|H|AB1|E|P|V|1|"? I don't know if this matches the naming in the client's business domain. The "Campo Key" term meta field name points in this direction, but it could also be like this to avoid confusing users with other names (e.g., "Campo Key Tuple").
Long story short, let's keep everything as-is, but in the future, we could rename everything from "HIS code", hisCode, or HIS_CODE to "Campo Key", campoKey, or CAMPO_KEY to prevent confusion. An outside developer probably doesn't know that "HIS code" and "Campo Key" is related on first glance.
Co-authored-by: Philipp Bammes <[email protected]>
I think there correct is still not clear for example the title of this ticket https://inpsyde.atlassian.net/browse/FAU-401 calls it "HIS-identifier", what should we call the individual members in term meta if "Campo keys" can be considered the whole |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature
What is the current behavior? (You can also link to an open issue here)
https://inpsyde.atlassian.net/browse/FAU-414
What is the new behavior (if this is a feature change)?
Added support for campo keys
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No
Related PRs:
idsattribute to degree programs overview shortcode FAU-Studium-Embed#31Other information: